Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add CI, fix existing warnings, add other improvements #1095

Merged
merged 36 commits into from
Mar 21, 2023

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Nov 8, 2022

Description of proposed changes

I surfaced warnings in the docs build via CI and fixed them. Also added some additional touch-ups from going through these docs files.

Related issue(s)

Testing

Checklist

  • Add a message in CHANGES.md summarizing the changes in this PR. Keep headers and formatting consistent with the rest of the file.

@victorlin victorlin requested a review from a team November 8, 2022 22:04
@victorlin victorlin self-assigned this Nov 8, 2022
Copy link
Member

@tsibley tsibley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to get these checked and fixed!

docs/conf.py Outdated
Comment on lines 94 to 98
# Don't check for type references in docstrings.
nitpick_ignore_regex = [
('py:.*', '.*'),
]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This smells stinky to me. It's very broad, and the error messages we're getting (as pasted in the commit message) don't seem to make sense. For example:

docstring of augur.align.prepare:1: WARNING: py:class reference target not found: The existing alignment filename

Why is the string The existing alignment filename being used as a reference target (e.g. a link destination)?

Something's fishy here. Is the sphinx.ext.napoleon conversion (63297d4) doing something bogus? Are a bunch of our docstrings using the wrong syntax for describing parameters?

I think it's worth tracking down what's going on here, as I suspect papering over it now will only cause us more headache in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've replaced 8005577 with several fixes: 0c3cb34...4375322

This ended up being a doozy, but I learned lots about how these docs are generated!

docs/faq/metadata.md Outdated Show resolved Hide resolved
augur/distance.py Outdated Show resolved Hide resolved
docs/usage/cli/distance.rst Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (87313dd) 68.25% compared to head (8e3705d) 68.25%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1095   +/-   ##
=======================================
  Coverage   68.25%   68.25%           
=======================================
  Files          63       63           
  Lines        6772     6772           
  Branches     1659     1659           
=======================================
  Hits         4622     4622           
  Misses       1841     1841           
  Partials      309      309           
Impacted Files Coverage Δ
augur/align.py 89.42% <ø> (ø)
augur/ancestral.py 66.94% <ø> (ø)
augur/clades.py 90.75% <ø> (ø)
augur/dates/__init__.py 90.78% <ø> (ø)
augur/distance.py 55.48% <ø> (ø)
augur/export_v2.py 68.74% <ø> (ø)
augur/filter/include_exclude_rules.py 97.27% <ø> (ø)
augur/filter/io.py 82.05% <ø> (ø)
augur/filter/subsample.py 98.76% <ø> (ø)
augur/frequency_estimators.py 36.16% <0.00%> (ø)
... and 15 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@victorlin victorlin force-pushed the victorlin/docs-ci branch 3 times, most recently from d383e35 to ae81fdd Compare January 19, 2023 23:16
@victorlin victorlin force-pushed the victorlin/docs-ci branch 3 times, most recently from 31b0f41 to 9211721 Compare March 9, 2023 00:37
This uses a reusable workflow in the nextstrain/.github repo.
rST is our standard doc format; Markdown is the legacy format.

Initial conversion performed with:

    pandoc -f markdown-smart -t rst-smart docs/faq/metadata.md > docs/faq/metadata.rst

and then I hand reviewed and made additional edits.

This fixes a broken link to https://docs.nextstrain.org/en/latest/guides/bioinformatics/lat_longs.html.
This was causing a warning when building the docs:

    Tests
    -----
    None:8: CRITICAL: Unexpected section title.

Removing since docstrings in other files do not have this heading.
In CLI pages with multiple headings.
Replace the literal blocks (introduced by the :: markers) with
code-block directives for nice syntax highlighting.
Fixes this warning:

    augur/io/__init__.py:docstring of augur.io:1: WARNING: duplicate object description of augur.io, other instance in api/developer/augur.io, use :noindex: for one of them

While :noindex: for Sphinx in general disables cross-referencing¹ and it
is briefly mentioned in the "Options and advanced usage" section of
automodule², there aren't any references anyways (as demonstrated by a
lack of related warnings/errors).

However, it's still unclear to me exactly why this shows on augur.io
only. I assume it has something to do with the re-exporting of public
API functions, which is currently specifi to augur.io.

¹ https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#basic-markup
² https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#directives
The section headings resulted in warnings:

    None:8: CRITICAL: Unexpected section title.

    Comparison methods
    ==================
    None:36: CRITICAL: Unexpected section title.

    Distance maps
    =============

They rendered fine in the Development API page¹ but did not render on
the CLI page².

This seems like a bug with sphinx-argparse³. For now, use bold text to
avoid the warnings and allow the text to render on the CLI page.

¹ https://docs.nextstrain.org/projects/augur/en/stable/api/developer/augur.distance.html
² https://docs.nextstrain.org/projects/augur/en/stable/usage/cli/distance.html
³ sphinx-doc/sphinx-argparse#31
This makes local builds show the same warnings in CI without erroring.
Without this prefix, a warning appears:

    WARNING: py:attr reference target not found: ValidationMode.SKIP
The majority of the codebase uses numpydoc for type annotations.
However, there are some usages of the newer PEP 484 type hints.

For example, augur.dates.get_numerical_dates has `metadata:pd.DataFrame`
in the function signature.

Without sphinx-autodoc-typehints, this caused a warning:

    augur/augur/dates.py:docstring of augur.dates.get_numerical_dates:1: WARNING: py:class reference target not found: pandas.core.frame.DataFrame

This commit fixes that warning.
This enables proper reference resolution during the docs build, and
provides useful links on the generated doc pages.
Without these pages, there would be warnings such as:

    WARNING: py:class reference target not found: AugurError
    WARNING: py:class reference target not found: DataErrorMethod

It's a good idea to have these doc pages regardless.
np.isscalar will return true for a float, but that is not valid input
for np.linspace(num).

Instead, allow native or numpy integers.
Otherwise the doctest gets parsed as numpydoc.
Better than the previous commit since this section is made for doctest examples¹.

¹ https://numpydoc.readthedocs.io/en/v1.5.0/format.html#examples
The bracket notation is suggested by PyCharm¹ but is not standard
numpydoc.

¹ https://www.jetbrains.com/help/pycharm/type-syntax-for-docstrings.html
"label" is not a valid type; it should be "str".
These resulted in nitpick "reference target not found" warnings.
Other register_parser functions don't have a docstring, and this one is
out of date.
These were causing nitpick warnings:

    WARNING: py:class reference target not found: TYPE
Bio.Phylo is the module, not the tree class.

This could instead be something more specific like
Bio.Phylo.Newick.Tree, but I opted for the more generic type.
`Phylo.Node` is not a valid class. I believe the intention is
`Bio.Phylo.BaseTree.Clade`, since that is the type of
`Bio.Phylo.BaseTree.Tree.root`
This class lives at pandas.io.parsers.TextFileReader, however, there are
no public docs for it and it isn't linked on pandas doc pages either¹.

Use single backticks to disable linking while denoting that it is a class².

¹ https://pandas.pydata.org/pandas-docs/version/1.4/reference/api/pandas.read_csv.html
² https://numpydoc.readthedocs.io/en/latest/format.html#common-rest-concepts
@victorlin
Copy link
Member Author

I spot-checked the preview build and it looks good. Merging this to start checking docs in other CI runs. Post-merge feedback is still welcome.

@victorlin victorlin merged commit 6f85cf4 into master Mar 21, 2023
@victorlin victorlin deleted the victorlin/docs-ci branch March 21, 2023 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Check docs build in CI
2 participants